Update hitch support.#9897
Conversation
There was a problem hiding this comment.
Pull request overview
Updates wolfSSL's hitch support from version 1.7.3 to 1.8.0, re-enabling the previously disabled CI/CD workflow.
Changes:
- Bumps the hitch version reference from 1.7.3 to 1.8.0 in the CI workflow
- Adds
CERTIFICATE_STATUS_REQUESTandWC_NO_STATIC_ASSERTflags to the hitch build configuration - Updates the hitch build steps to use the new patch file and
./bootstrapinstead ofautoreconf
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
configure.ac |
Adds CSR and static assert suppression flags needed for hitch 1.8.0 compatibility |
.github/workflows/disabled/hitch.yml |
Updates CI to target hitch 1.8.0, adjusts build steps and ignored tests |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@kareem-wolfssl — friendly ping: @julek-wolfssl's review comment from 2026-03-06 is the only blocker here. It's a one-line refactor: use Landing this would unblock wolfSSL/osp#325 and resolve wolfSSL/osp#218. |
|
Opened kareem-wolfssl#4 against the Once @kareem-wolfssl merges that into |
Address julek-wolfssl's review on wolfSSL#9897: replace the two remaining hardcoded 1.8.0 occurrences with ${{ matrix.ref }} so the matrix entry is the single source of truth for the hitch version under test.
Address julek-wolfssl's review on wolfSSL#9897: replace the two remaining hardcoded 1.8.0 occurrences with ${{ matrix.ref }} so the matrix entry is the single source of truth for the hitch version under test.
|
Retest this please |
|
Updated. OSP PR needs to be merged first so the re-enabled hitch test can work correctly. (wolfSSL/osp#325) |
|
FYI: Not ready for merge just yet as the Github hitch test is still having an issue with including wolfSSL settings. |
|
This now depends on wolfSSL/osp#338 being merged to fix the warnings in the Github runner hitch build. |
|
Retest this please |
Address julek-wolfssl's review on wolfSSL#9897: replace the two remaining hardcoded 1.8.0 occurrences with ${{ matrix.ref }} so the matrix entry is the single source of truth for the hitch version under test.
Address julek-wolfssl's review on wolfSSL#9897: replace the two remaining hardcoded 1.8.0 occurrences with ${{ matrix.ref }} so the matrix entry is the single source of truth for the hitch version under test.
Requires wolfSSL/osp#325. Fixes wolfSSL/osp#218.
Address julek-wolfssl's review on wolfSSL#9897: replace the two remaining hardcoded 1.8.0 occurrences with ${{ matrix.ref }} so the matrix entry is the single source of truth for the hitch version under test.
…fine WC_NO_STATIC_ASSERT while not building the library to avoid including assert.h in external applications like hitch.
dgarske
left a comment
There was a problem hiding this comment.
Skoll Code Review
Scan type: reviewOverall recommendation: COMMENT
Findings: 5 total — 4 posted, 1 skipped
4 finding(s) posted as inline comments (see file-level comments below)
Posted findings
- [Medium] wc_static_assert silently becomes a no-op for all external applications, not just hitch —
wolfssl/wolfcrypt/types.h:2189-2192 - [Medium] Two additional hitch tests ignored without explanation —
.github/workflows/hitch.yml:47-49 - [Low] testsuite translation unit silently loses the test.h static assert —
wolfssl/wolfcrypt/types.h:2189-2192 - [Low] Ambiguous 'CSR' abbreviation in configure.ac comment —
configure.ac:9120
Skipped findings
- [Medium]
Workflow depends on unmerged osp PR #325 (hitch_1.8.0.patch) -- merge ordering required
Review generated by Skoll
|
|
||
| #define WC_CPP_CAT4_(a, b, c, d) a ## b ## c ## d | ||
| #define WC_CPP_CAT4(a, b, c, d) WC_CPP_CAT4_(a, b, c, d) | ||
| #if !defined(BUILDING_WOLFSSL) && !defined(WOLFSSL_VIS_FOR_TESTS) && \ |
There was a problem hiding this comment.
🟠 [Medium] wc_static_assert silently becomes a no-op for all external applications, not just hitch
The new gate defines WC_NO_STATIC_ASSERT by default whenever BUILDING_WOLFSSL and WOLFSSL_VIS_FOR_TESTS are absent, which is every downstream application build. This is a deliberate change (commit bc8c93e: avoid pulling \<assert.h> into apps like hitch) and I verified the mechanics are sound: BUILDING_WOLFSSL is confined to the library target flags (src/include.am:47-48) so it does not leak into the generated options.h; in-tree test apps define WOLFSSL_VIS_FOR_TESTS before any wolfSSL include (tests/unit.h:26, wolfcrypt/test/test.c:31), so the dedicated compile-time assert tests in tests/api/test_mldsa_legacy.c stay live; and the no-op expansion struct wc_static_assert_dummy_struct; is warning-clean even when repeated consecutively at file scope as in error-crypt.h:344-346 (verified empirically with gcc/g++ -Wall -Wextra -Wredundant-decls -Wpedantic -std=c89). However, two consequences are worth surfacing: (1) any downstream application that deliberately uses the public wc_static_assert/wc_static_assert2 macros for its own compile-time checks (the documented pattern in doc/dilithium-to-mldsa-migration.md and tests/api/test_mldsa_legacy.c) now silently loses enforcement unless it defines WC_USE_STATIC_ASSERT -- the asserts compile but check nothing, which is the worst failure mode for an assert; (2) the header-level configuration sanity asserts (wolfssl/error-ssl.h:252, wolfssl/wolfcrypt/error-crypt.h:344-346, wolfssl/wolfcrypt/sp_int.h:987-988) no longer fire in application translation units, so a user_settings.h mismatch is only caught when the…
Fix: Add an explanatory comment above the new block (the change is non-obvious and the rationale currently lives only in a commit message), and document WC_USE_STATIC_ASSERT / WC_NO_STATIC_ASSERT semantics wherever build macros are documented, so downstream users who rely on wc_static_assert know the default flipped.
| # List of releases to test | ||
| include: | ||
| - ref: 1.7.3 | ||
| - ref: 1.8.0 |
There was a problem hiding this comment.
🟠 [Medium] Two additional hitch tests ignored without explanation
The ignore list grows from 3 to 5 tests: test12-cfg.sh and test41-client-cert-pb.sh are newly skipped for hitch 1.8.0, in addition to the previously skipped test13-r82.sh, test15-proxy-v2-npn.sh, and test39-client-cert-proxy.sh. There is no comment in the workflow (and nothing in the PR description -- the Testing section is empty) explaining why each test is incompatible with the wolfSSL build, whether the failures are expected limitations (e.g. missing NPN, proxy-v2 differences) or temporary gaps that should be tracked. test41-client-cert-pb.sh in particular is a client-cert path; together with the already-skipped test39-client-cert-proxy.sh this means client-certificate behavior through hitch gets thinner CI coverage, and future maintainers have no way to know if these can be re-enabled.
Fix: Add a short comment per newly ignored test stating why it is skipped (known incompatibility vs. tracked gap), or reference the osp issue that documents them, so the list does not silently grow over releases.
|
|
||
| #define WC_CPP_CAT4_(a, b, c, d) a ## b ## c ## d | ||
| #define WC_CPP_CAT4(a, b, c, d) WC_CPP_CAT4_(a, b, c, d) | ||
| #if !defined(BUILDING_WOLFSSL) && !defined(WOLFSSL_VIS_FOR_TESTS) && \ |
There was a problem hiding this comment.
🔵 [Low] testsuite translation unit silently loses the test.h static assert
testsuite/testsuite.c includes wolfcrypt/test/test.h (which contains wc_static_assert(-(long)MIN_CODE_E < 0x7ffL); at test.h:65) but defines neither BUILDING_WOLFSSL nor WOLFSSL_VIS_FOR_TESTS, so with this PR that assert expands to the no-op dummy struct in the testsuite translation unit. This is benign -- the identical assert is still evaluated when wolfcrypt/test/test.c (which defines WOLFSSL_VIS_FOR_TESTS at line 31) is compiled in the same build -- but it illustrates that the gate also reaches in-tree consumers beyond external applications. Noting for completeness; no action strictly required.
Fix: Optional: define WOLFSSL_VIS_FOR_TESTS in testsuite.c for consistency with the other in-tree test drivers.
| ENABLED_OCSP="yes" | ||
| fi | ||
|
|
||
| # Requires CSR for wolfSSL_set_tlsext_status_ocsp_resp |
There was a problem hiding this comment.
🔵 [Low] Ambiguous 'CSR' abbreviation in configure.ac comment
The comment # Requires CSR for wolfSSL_set_tlsext_status_ocsp_resp uses 'CSR' to mean Certificate Status Request, but in TLS/PKI code 'CSR' overwhelmingly reads as Certificate Signing Request. The change itself is correct and was verified: wolfSSL_set_tlsext_status_ocsp_resp is compile-gated behind HAVE_CERTIFICATE_STATUS_REQUEST/_V2 (src/ssl_api_crl_ocsp.c:524), the option processing at configure.ac:8047 runs before the hitch block so the manual AM_CFLAGS additions are required (exactly mirroring the established pattern at configure.ac:9473-9477), and OCSP is already force-enabled just above with its define consumed later at line 11708. Only the comment wording is worth tightening.
Fix: Spell out 'certificate status request (OCSP stapling)' in the comment to avoid confusion with Certificate Signing Request.
Description
Update hitch support for the latest release of hitch, 1.8.0.
Re-enable hitch CI/CD test support.
Requires wolfSSL/osp#325.
Fixes wolfSSL/osp#218
Testing
How did you test?
Checklist